Code & Security Review

Nicely scoped change — the UI now branches on capability instead of provider identity, the resolver-injection pattern mirrors the existing onMarkInReview / onListWorkspaceRepos closures (CrowUI stays free of a CrowProvider dependency), and the new tests cover the wiring contract well. Verified locally: BackendsTests 15/15 and the new AppStateCapabilityTests 5/5 pass.

There is one issue that should be fixed in this round.

Code Quality

🟡 Yellow — Capability contract is now self-contradictory across the codebase

This PR redefines what .projectBoardStatus means (from "setTaskStatus is callable and succeeds" → "the provider supports project-board status as a UI concept") and updates GitHubTaskBackend.swift + BackendsTests.swift accordingly. But the authoritative contract docs that encode the old, strict meaning are left untouched and now directly contradict the implementation:

• Packages/CrowProvider/Sources/CrowProvider/TaskBackend.swift:33-36 — still says: "Capability-gated: only call when capabilities.contains(.projectBoardStatus). Calling without the capability throws ProviderError.unimplemented." The clear implication (call it with the capability and it works) is now false for GitHubTaskBackend.
• Packages/CrowProvider/Sources/CrowProvider/TaskBackend.swift:45-47 — "Gates setTaskStatus. Without this capability that method throws." Now GitHubTaskBackend declares the capability and throws.
• docs/adr/0005-task-and-code-backend-protocols.md:31-37 — the canonical example is if taskBackend.capabilities.contains(.projectBoardStatus) { try await taskBackend.setTaskStatus(...) }, described as "the direct replacement for if session.provider == .github." A caller following that exact documented pattern now hits .unimplemented.

This is the precise hazard the comment removed in this PR warned about ("declaring it means a capability-gated caller can call setTaskStatus and expect success. We add the flag when the implementation arrives, not before."). It is not a live bug — the only runtime path routes through onMarkInReview → IssueTracker.markInReview, and no caller gates setTaskStatus on the capability today — so functionally the PR is sound. But the capability now means two contradictory things depending on which file you read, and the next person who migrates markInReview (or adds any capability-gated setTaskStatus caller) will trust the protocol/ADR contract and get a thrown error.

Suggested fix (cheap, same round trip): update the TaskBackend.swift doc comments (lines 33-36 and 45-47) and the ADR 0005 example to reflect the loosened "UI surface" meaning, and note that setTaskStatus callability is not implied by the flag until the GraphQL migration lands. (Alternatively, if the flag is meant to stay strict, gate the UI on a distinct concept instead of overloading .projectBoardStatus — but doc alignment is the lighter path.)

Security Review

Strengths:

• No new attack surface. Change is a UI-gating predicate plus a synchronous capability lookup; no new shell-outs, network calls, input parsing, or credential handling.
• Resolver defaults to nil → false, so unwired contexts (tests, previews) safely hide the action rather than failing open.

Concerns: None.

Notes (non-blocking, no action required)

• 🟢 AppDelegate.swift:718 captures [providerManager] strongly while the sibling closures use [weak tracker]. Fine here — ProviderManager is a leaf Sendable factory that does not reference appState, so no retain cycle — just noting the deliberate asymmetry.

Summary Table

Recommendation: Request Changes — driven by [0 Red, 1 Yellow, 1 Green] findings. The code is correct and tested; the blocker is the protocol/ADR contract docs now contradicting the redefined capability semantics. Align the docs and this is good to merge.

🐦‍⬛ Reviewed by Crow via Claude Code

Code & Security Review

Critical Issues

None.

Security Review

Strengths:

• No new external input, network calls, secrets, or shell construction. The change is a pure indirection of an existing UI predicate.
• Resolver guards session.provider != nil before any lookup; taskBackend(for:) is total over the Provider enum and non-throwing, so there is no crash/throw path from the UI.

Concerns:

• None.

Code Quality

• Behavioral equivalence verified. Only GitHubTaskBackend declares .projectBoardStatus; GitLabTaskBackend and StubCorveilTaskBackend declare empty capability sets. So appState.canSetProjectStatus(for:) is true for exactly the sessions the prior session.provider == .github guard covered (SessionDetailView.swift:176, SessionListView.swift:303).
• No retain cycle. The resolver captures [providerManager] (a let on AppDelegate), not self. ProviderManager.taskBackend(for:) is nonisolated, so it is synchronously callable from the MainActor resolver — no await, no actor hop.
• Loosened capability contract is safe. .projectBoardStatus now means "UI surface supported" rather than "setTaskStatus is wired." The only capability consumers are the two UI guards; no production code calls setTaskStatus while gating on the capability and expecting success, so the GitHub backend's continued .unimplemented throw breaks no live caller. Execution still routes through onMarkInReview → IssueTracker.markInReview. The weakening is documented consistently across TaskBackend.swift, GitHubTaskBackend.swift, and ADR 0005, and pinned by testGitHubTaskBackendSetTaskStatusThrowsUnimplemented.
• Doc reference IssueTracker.swift:2549 is accurate (inside markInReview).

Verification

• swift test --package-path Packages/CrowCore --filter AppStateCapability → 5/5 pass.
• swift test --package-path Packages/CrowProvider --filter BackendsTests → 15/15 pass (flipped testGitHubTaskBackendDeclaresCapabilities assertion green; unimplemented-throw assertion still green).
• Full-repo swift build failed only on a missing GhosttyKit.xcframework binary artifact — a local environment issue unrelated to this PR; the touched packages compile and test cleanly.

Summary Table

Recommendation: Approve — driven by [0 Red, 0 Yellow, 0 Green] findings. Behaviorally equivalent, well-tested, and thoroughly documented; the deferred GraphQL migration is correctly scoped out and the legacy execution path is preserved.

🐦‍⬛ Reviewed by Crow via Claude Code